Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dlt-user: fix crash with certain strings #463

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

alexmohr
Copy link
Contributor

@alexmohr alexmohr commented Apr 5, 2023

make sure that a string with exactly UINT16_MAX bytes does not overflow the dlt buffer

Given the example below there is a crash is libdlt

#include <dlt/dlt.h>
#include <cstring>
#include <string>

DLT_DECLARE_CONTEXT(mycontext)

int main()
{
    DLT_REGISTER_APP("FOOB", "TEST DEMO");
    DLT_REGISTER_CONTEXT(mycontext, "CTX1", "Test Context for Logging");
    size_t len = 65535;
    char* txt = (char*)malloc(len);
    memset(txt, 'c', len);
    
    // will crash
    DLT_LOG(mycontext, DLT_LOG_INFO, DLT_STRING(txt));
}

The expansion of the macro looks like this

do {
    DltContextData log_local;
    int dlt_local;
    dlt_local = dlt_user_log_write_start(&mycontext, &log_local, DLT_LOG_INFO);
    if (dlt_local == DLT_RETURN_TRUE) {
        (void)dlt_user_log_write_string(&log_local, txt);
        (void)dlt_user_log_write_finish(&log_local);
    }
} while (0

The crash now happens due to the following chain of events.
dlt_user_log_write_start(&mycontext, &log_local, DLT_LOG_INFO); is calling dlt_user_log_write_string_utils_attr
Which is casting the string length into uint16_t. This is fine as our string is exactly as long as uint16_t allows.
Now dlt_user_log_write_sized_string_utils_attr is called.
This sets uint16_t arg_size = (uint16_t) (length + 1). Which will overflow the uint16_t and arg_size will be 0.

// new_log_size = 2
// will be increment to 6 if verbose mode is on
size_t new_log_size = log->size + arg_size + sizeof(uint16_t);

The code above also won't detect that the string is too large as the uin16_t also overflows and the sum will be much smaller as dlt_user.log_buf_len resulting in the truncation check to eval to false.

/* Check log size condition */
if (new_log_size > dlt_user.log_buf_len)
// no jump into if body as new_log_size is small enough to fit into the buffer
// thus no truncation

Now log->size is incremented in several places.
In my example with enabled verbose mode it's 6

 /* Whole string will be copied */
memcpy(log->buffer + log->size, text, length);

The code above is now copying into the buffer at buffer + log->size.
As the original length with 65535 is used we're writing 6 bytes behind the actual buffer.
With larger strings, their length is still a multiple of uint16_t max, the same crash occurred.
If it's not a multiple the application does not crash but reads a certain amount of bytes from the buffer, truncating the log without any message that it was cut of.

It's fixed by using size_t for the calculation, so the truncation is working properly.

After the fix case DLT_RETURN_USER_BUFFER_FULL: is used and the truncated string will be logged.
The program was tested solely for our own use cases, which might differ from yours.
Licensed under Mozilla Public License Version 2.0

Alexander Mohr, [email protected], Mercedes-Benz Tech Innovation GmbH, imprint

make sure that a string with exactly UINT16_MAX bytes does not
overflow the dlt buffer

Signed-off-by: Alexander Mohr <[email protected]>
@alexmohr
Copy link
Contributor Author

alexmohr commented Apr 5, 2023

Another way to test the faulty version is by running dlt-example-user

 dlt-example-user -n 1 $(cat /dev/urandom | tr -dc '[:alpha:]' | fold -w ${1:-65535} | head -n 1)

Copy link
Collaborator

@michael-methner michael-methner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Local verification successful

@michael-methner michael-methner merged commit 9a2312d into COVESA:master Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants